Repository navigation
feat(librpa): add reader-v1 producer exports - #8053
Conversation
mohanchen
added
the
Features Needed
…der-v1-remote-update # Conflicts: # .readthedocs.yaml # docs/conf.py
…ssion Preserve velocity_matrix.txt, rename Vxc text output and references, reject unsupported reader-v1 symmetry, abort the producer communicator on output exceptions, and validate two-rank KS assembly against same-run text data. Drop unrelated RTD changes already in develop and remove unused gauge references.
Critsium-xy
left a comment
Critsium-xy
left a comment
There was a problem hiding this comment.
Re-review of da7d65329. Findings 1-9, 12 and 13 from the previous round are resolved. One blocking regression in the new INPUT guard is inline below; it is what the failing Test job is reporting.
Two earlier points are still open, both in the PR description:
- The
vxc_out.dat->vxc.txtrename is a user-visible break and is not mentioned, andwrite_orb_energystill carries theterm == "vxc"special case while the other terms keep<term>_out.dat. - The k-point changes are not mentioned either:
kvec_c_fullis now the global full mesh on every rank andibz_indexis no longer overwritten by the identity map after reduction, which also fixes an out-of-bounds read inewald_vq.hpp:121. This affects EXX and has no EXX-side test.
Also: the summary still names the parameter out_librpa_reader_version instead of out_librpa_ver, and does not state the new hard requirement that out_librpa_ver = 1 needs a uniform Monkhorst-Pack grid.
Minor: esolver_fp.cpp drops from 58 to 56 in code_quality_score.py (already under the 60 line before this PR). Extracting the guard into a helper would keep LibRPA-specific validation out of the generic FP entry point.
Not built or run locally; based on static review plus the CI Test job log (run 37887346672).
|
|
esolver_fp.cpp serves as the base class, meaning it should contain the knowledge required for first-principles calculations. This class should not know about any physical quantities related to basis sets, or even orbitals. "fp" stands for first-principles. esolver_ks.cpp inherits from esolver_fp.cpp and only then acquires the concept of electronic orbitals. esolver_of.cpp, which sits parallel to esolver_ks.cpp, does not need to know anything about electronic orbitals, so esolver_fp.cpp must not contain psi in principle. The source_lcao/module_ri/rpa_lri_input.h you added should not appear here; I suggest moving it to esolver_ks_lcao.cpp instead. This is again a dependency problem: if you want to add something to k-points, it should strictly belong to k-points and have nothing to do with librpa. It is a feature added to k-points, and librpa should depend on k-points, not the other way around. Otherwise you end up with inverted dependencies — for instance, the OFDFT code would end up depending on librpa, which creates severe entanglement in the codebase. |
|
LGTM now, thanks for your continue updating of the codes. |
mohanchen
added
Tests/Examples
4 participants
Summary
OUT.librpa.nspin = 2cases.out_librpa_ver(0= legacy output,1= reader-v1).out_librpa_ver = 1requires a uniform Monkhorst-Pack k-point grid and supports symmetry-reduced grids; explicit or nonuniform KPT lists are rejected before SCF.vxc_out.dattovxc.txt(and the corresponding local/EXX files tovxc_local.txt/vxc_exx.txt). This is a user-visible filename change; other orbital-energy terms retain<term>_out.dat.kvec_c_fullmesh on every rank and keep the reducedibz_indexmapping. This fixes the full-grid access inewald_vq.hpp:121and changes shared k-point semantics used by EXX; this PR has no dedicated EXX-side regression.